-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly update environment from the importing plan #2452
Conversation
Weird thing is that this is being tested: tmt/tests/plan/import/modify-data/plans.fmf Lines 8 to 16 in 0b95455
|
Thank @LecrisUT looking into why it was not caught |
Oh, I have an idea. Could it be that when environment is treated inside defining the plan/test is different from when it is used for execute, i.e. if it's used as replacing a variable in |
Yes, seems that the explicit node data expansion handles correctly the case when variable is in the config: Lines 2345 to 2348 in 722563c
But its value does not get to the actual environment when an external script is executed. |
The fix seems to be ok. Although, when reading the code I realized that in a76772f we've introduced double-updating the environment from the command line. This does not hurt but let's get that straight: I've filed #2461 to track this.
Yes, please cover the scenario when the variable is used in an external script, not directly in the |
@happz so I moved back from
|
@psss I am reading now your comment, I believe my test is enough, even though I am not sure why I added it to Let me know if you want more proof it works as expected, I believe it does catch the problem, when I run it with old code, my test fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks good. Thanks for the fix. I just remembered we've agreed on the hacking session that we could use an environment
property which could also cover the #2461 but let's handle that one separately.
I've confirmed that the changes here work as intended |
The environment from the importing plan was ignored. Resolves teemtee#2446 Signed-off-by: Miroslav Vadkerti <[email protected]> Co-authored-by: Petr Šplíchal <[email protected]>
/packit test --identifier full |
The environment from the importing plan was ignored.
As agreed, let's make the
environment
a cached property and inheritthe importing plan environment automatically.
Resolves #2446
Pull Request Checklist